Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ask users for a pin when interacting with locks/garage doors #23223

Merged
merged 2 commits into from Apr 19, 2019
Merged

Ask users for a pin when interacting with locks/garage doors #23223

merged 2 commits into from Apr 19, 2019

Conversation

balloob
Copy link
Member

@balloob balloob commented Apr 19, 2019 •
edited

Breaking Change:

The allow_unlock option has been removed for both Cloud and manual Google Assistant installations. Instead, you need to define a pin. Google Cloud users can do this in the cloud preferences UI. Manual installation will have to add secure_devices_pin to their config.

Description:

This replaces the allow_unlock toggle with the official approach to interacting with secure devices via Google Assistant: 2FA using a pin challenge.

This change has been requested by Google hence tagged for 0.92.

Related issue (if applicable): fixes #23219

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@balloob balloob requested a review from a team as a code owner April 19, 2019 03:23
@ghost
Copy link

ghost commented Apr 19, 2019

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (cloud) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

@balloob balloob mentioned this pull request Apr 19, 2019
Merged
@balloob balloob added this to the 0.92.0 milestone Apr 19, 2019
@codecov
Copy link

codecov bot commented Apr 19, 2019

Codecov Report

Merging #23223 into dev will decrease coverage by <.01%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #23223      +/-   ##
==========================================
- Coverage   94.17%   94.17%   -0.01%     
==========================================
  Files         457      457              
  Lines       37095    37126      +31     
==========================================
+ Hits        34935    34962      +27     
- Misses       2160     2164       +4
Impacted Files Coverage Δ
homeassistant/components/cloud/client.py 87.25% <ø> (ø) ⬆️
...meassistant/components/google_assistant/helpers.py 96.15% <100%> (+0.11%) ⬆️
homeassistant/components/cloud/http_api.py 99% <100%> (-0.01%) ⬇️
homeassistant/components/cloud/prefs.py 98.11% <100%> (ø) ⬆️
homeassistant/components/google_assistant/http.py 94.28% <100%> (+0.16%) ⬆️
homeassistant/components/cloud/const.py 100% <100%> (ø) ⬆️
...ssistant/components/google_assistant/smart_home.py 91.2% <100%> (ø) ⬆️
homeassistant/components/google_assistant/const.py 100% <100%> (ø) ⬆️
homeassistant/components/google_assistant/error.py 92.3% <88.88%> (-7.7%) ⬇️
homeassistant/components/google_assistant/trait.py 96.03% <89.28%> (-0.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2b4e24...ac42076. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Apr 19, 2019 •
edited

Codecov Report

Merging #23223 into dev will decrease coverage by <.01%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #23223      +/-   ##
==========================================
- Coverage   94.17%   94.17%   -0.01%     
==========================================
  Files         457      457              
  Lines       37095    37126      +31     
==========================================
+ Hits        34935    34962      +27     
- Misses       2160     2164       +4
Impacted Files Coverage Δ
homeassistant/components/cloud/client.py 87.25% <ø> (ø) ⬆️
...meassistant/components/google_assistant/helpers.py 96.15% <100%> (+0.11%) ⬆️
homeassistant/components/cloud/http_api.py 99% <100%> (-0.01%) ⬇️
homeassistant/components/cloud/prefs.py 98.11% <100%> (ø) ⬆️
homeassistant/components/google_assistant/http.py 94.28% <100%> (+0.16%) ⬆️
homeassistant/components/cloud/const.py 100% <100%> (ø) ⬆️
...ssistant/components/google_assistant/smart_home.py 91.2% <100%> (ø) ⬆️
homeassistant/components/google_assistant/const.py 100% <100%> (ø) ⬆️
homeassistant/components/google_assistant/error.py 92.3% <88.88%> (-7.7%) ⬇️
homeassistant/components/google_assistant/trait.py 96.03% <89.28%> (-0.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2b4e24...ac42076. Read the comment docs.

pvizeli
pvizeli approved these changes Apr 19, 2019
View reviewed changes
Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

google_secure_devices_pin is very long, maybe google_devices_pin would be enough to hold the code short

@balloob balloob merged commit 0533f56 into dev Apr 19, 2019
@delete-merged-branch delete-merged-branch bot deleted the google-ass-challenge branch April 19, 2019 21:50
balloob added a commit that referenced this pull request Apr 20, 2019
@balloob
Ask users for a pin when interacting with locks/garage doors (#23223)
d0c6f0b 
* Ask users for a pin when interacting with locks/garage doors

* Deprecate allow_unlock option
@FutureTense
Copy link

FutureTense commented Apr 29, 2019 •
edited

This change has been requested by Google hence tagged for 0.92.

@balloob is google requiring this change? While the feature is nice to have, I’m not a fan of being forced to use a pin. Is there a way to make the pin optional?

Regardless, how is one supposed to use the pin? What is the syntax to unlock “Front Door” with pin 1234?

OK Google, unlock the front door 1234

@brianjamescarter
Copy link

has anyone got this to work? my google just asks for the code then says, "I looked but this can't be played at this time"

this is beyond frustrating thanks google

@brmo
Copy link

brmo commented May 6, 2019

I got it to work just by adding my cover.garage_door entity to the include_entities section. However even after setting a pin, it never asks me for it, but rather carries on doing the action. Security flaw there.

@MartinHjelmare
Copy link
Member

Please open an issue if you suspect a bug. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support or bug reports. Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators May 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pvizeli pvizeli pvizeli approved these changes

Assignees
No one assigned
Labels
breaking-change cherry-picked cla-signed core has-tests integration: cloud integration: google_assistant
Projects
None yet
Milestone
0.92.0
Development

Successfully merging this pull request may close these issues.

Google Assistant: Add 2FA to doors, locks
7 participants
@balloob @FutureTense @brianjamescarter @brmo @MartinHjelmare @pvizeli @homeassistant